Checkmarx One – Scan Summary & Details – e12b6438-4de2-4172-932f-c643de4da5dd

Great job! No new security vulnerabilities introduced in this pull request

Codecov Report

❌ Patch coverage is 91.47727% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.02%. Comparing base (95e329a) to head (c2b8ee1).

@@            Coverage Diff             @@
##             main     #494      +/-   ##
==========================================
+ Coverage   78.95%   79.02%   +0.07%     
==========================================
  Files         296      298       +2     
  Lines       30904    31080     +176     
==========================================
+ Hits        24400    24561     +161     
- Misses       6504     6519      +15     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Thank you for the review!

One more (optional) suggestion here would be to take test vectors from the TS clients and feed them into the unit tests so we know the implementation works the same way.

I looked around the clients codebase, but I don't see a full test vector for an end-to-end (PRF, User Key) -> RotateableKeySet and back. There are mocked values that don't verify the actual cryptography in https://github.com/bitwarden/clients/blob/main/libs/auth/src/common/login-strategies/webauthn-login.strategy.spec.ts and https://github.com/bitwarden/clients/blob/main/libs/common/src/auth/services/webauthn-login/webauthn-login.service.spec.ts. The Rust types guarantee some of the other invariants tested in. Did you have some specific test vectors in mind?

@iinuwa just a ping that the lint failed and needs a cargo +"nightly-2025-08-18" fmt

@iinuwa just a ping that the lint failed and needs a cargo +"nightly-2025-08-18" fmt

Thanks. I'm using jj which doesn't run git hooks, and my IDE formats according to Rust stable formatting rules, but doesn't format comments the same way. I'll work that out in my workflow.

Claude finished @iinuwa's task —— View job

Code Review: Add methods to create rotateable key sets from PRF

Summary of Changes Since Last Review

This PR adds functionality to create rotateable key sets from PRF (Pseudorandom Function) outputs for mobile unlock passkeys. The implementation consists of:

1. New PRF key derivation (prf.rs): Derives symmetric keys from PRF output using HKDF key stretching
2. New RotateableKeySet implementation (rotateable_key_set.rs): Manages key rotation using asymmetric key pairs
3. Client API methods: Exposes make_prf_user_key_set() through crypto client interfaces
4. Tests: Unit tests for both PRF derivation and key set operations

Recent commits addressed previous feedback:

• Clarified PRF input truncation behavior in documentation
• Fixed clippy lints and feature-gating issues
• Rebased on main branch

Critical Issues

1. Test Coverage Gap: Deterministic Test Data (⚠️ High Priority)

Location: crates/bitwarden-crypto/src/keys/prf.rs:45-51

The test test_long_prf_truncated_to_proper_length has a critical flaw:

fn test_long_prf_truncated_to_proper_length() {
    let long_prf = pseudorandom_bytes(33);
    let prf = pseudorandom_bytes(32);
    let key1 = derive_symmetric_key_from_prf(&long_prf).unwrap();
    let key2 = derive_symmetric_key_from_prf(&prf).unwrap();
    assert_eq!(key1, key2);  // This will always pass!
}

The pseudorandom_bytes() function generates identical sequences for any length starting from index 0:

• pseudorandom_bytes(33) → [0, 1, 2, ..., 31, 32]
• pseudorandom_bytes(32) → [0, 1, 2, ..., 31]

So the first 32 bytes are identical, making this test meaningless. It doesn't verify truncation behavior at all.

Fix: Use different test data to verify truncation:

#[test]
fn test_long_prf_truncated_to_proper_length() {
    let mut long_prf = pseudorandom_bytes(32);
    long_prf.push(255); // Add different byte at the end
    let prf = pseudorandom_bytes(32);
    let key1 = derive_symmetric_key_from_prf(&long_prf).unwrap();
    let key2 = derive_symmetric_key_from_prf(&prf).unwrap();
    assert_eq!(key1, key2);
}

2. Missing Test: PRF Boundary Conditions (⚠️ Medium Priority)

Location: crates/bitwarden-crypto/src/keys/prf.rs:19-57

The tests cover:

• ✅ 32-byte PRF (success case)
• ✅ All-zero PRF (failure case)
• ✅ Short PRF < 32 bytes (failure case)
• ⚠️ Long PRF > 32 bytes (flawed test, see above)

Missing test cases:

1. Exactly 33 bytes (minimum truncation case)
2. Much longer PRF (e.g., 64 bytes) to verify proper handling
3. PRF with non-zero trailing bytes that should be discarded

3. Potential Timing Attack in Zero-Check (⚠️ Medium Priority - Security)

Location: crates/bitwarden-crypto/src/keys/prf.rs:11-13

if secret.iter().all(|b| *b == b'\0') {
    return Err(CryptoError::ZeroNumber);
}

The Iterator::all() method uses short-circuit evaluation, which means:

• If the first byte is non-zero, it returns immediately
• If all bytes are zero, it checks all 32 bytes

This creates a timing side-channel that could theoretically leak information about how many leading zeros are in the PRF output.

Recommendation: Use constant-time comparison:

use subtle::ConstantTimeEq;

let is_zero = secret.ct_eq(&[0u8; 32]).into();
if is_zero {
    return Err(CryptoError::ZeroNumber);
}

Note: This may be overly cautious depending on your threat model. If the PRF comes from a hardware authenticator in a trusted environment, timing attacks may not be a practical concern. However, following CLAUDE.md guidance: "New crypto algorithms or key derivation - Detailed description, review and audit trail required."

4. Documentation: Missing Algorithm Rationale (⚠️ Medium Priority)

Location: crates/bitwarden-crypto/src/keys/prf.rs:1-17

Per CLAUDE.md: "New crypto algorithms or key derivation - Detailed description, review and audit trail required. Document algorithm choice rationale and test vectors."

The PRF derivation uses HKDF-based key stretching (stretch_key), but there's no documentation explaining:

• Why HKDF-SHA256 is appropriate for PRF outputs
• Why 32-byte truncation is cryptographically safe
• Reference to TypeScript implementation for verification
• Security assumptions about PRF entropy

Recommendation: Add module-level documentation:

//! # PRF Key Derivation
//!
//! Derives symmetric keys from WebAuthn PRF extension outputs.
//!
//! ## Algorithm
//! 1. Truncate PRF output to first 32 bytes
//! 2. Apply HKDF-SHA256 key stretching via `stretch_key()`
//!    - Derives enc_key: HKDF(prf, info="enc")
//!    - Derives mac_key: HKDF(prf, info="mac")
//! 3. Returns AES-256-CBC-HMAC-SHA256 key
//!
//! ## Security Properties
//! - PRF output is assumed to be cryptographically random
//! - 32 bytes (256 bits) provides sufficient entropy
//! - HKDF expansion ensures key independence
//!
//! ## Compatibility
//! Based on TypeScript implementation:
//! - https://github.com/bitwarden/clients/.../webauthn-login-prf-key.service.ts

Suggested Improvements

5. Inconsistent Error Handling (🎨 Improvement)

Location: crates/bitwarden-crypto/src/keys/prf.rs:8-9

The error message for short PRF is generic:

let (secret, _) = prf.split_at_checked(32).ok_or(CryptoError::InvalidKeyLen)?;

This returns CryptoError::InvalidKeyLen, which doesn't indicate:

• What the minimum length is (32 bytes)
• That this is a PRF-specific requirement
• What the actual length was

Suggestion: Consider adding a more specific error or improving documentation to help with debugging.

6. Dead Code with TODOs (🎨 Improvement)

Location: crates/bitwarden-crypto/src/keys/rotateable_key_set.rs:75-96

// TODO: Eventually, the webauthn-login-strategy service should be migrated
// to use this method, and we can remove the #[allow(dead_code)] attribute.
#[allow(dead_code)]
fn unlock<Ids: KeyIds>(...)

Observation: The unlock() method is intentionally unused, waiting for future migration from TypeScript clients. This is fine, but consider:

• Creating a tracking issue in Jira for the migration
• Referencing the issue number in the TODO comment
• Or removing the method until it's actually needed (YAGNI principle)

Same applies to: rotate_key_set() function at line 98-123

7. Potential UX Improvement: PRF Length Validation (💭 Consideration)

Location: crates/bitwarden-crypto/src/keys/prf.rs:7-17

The function accepts any PRF ≥ 32 bytes and silently truncates. Consider whether you want:

Option A (Current): Accept any length ≥ 32, truncate silently

• ✅ More flexible
• ❌ Silent truncation could hide bugs in calling code

Option B: Accept exactly 32 bytes, require caller to truncate

• ✅ More explicit about expectations
• ✅ Caller bugs are caught earlier
• ❌ Less convenient

Option C: Accept any length, but document/warn about truncation

• ✅ Balance between flexibility and safety
• ✅ Current documentation already mentions this

I think Option C (current approach) is reasonable, especially since the PR description mentions this is based on existing TypeScript code. Just ensure calling code is aware of the truncation behavior.

8. Test Quality: Use of expect() in Test Helper (🎨 Minor)

Location: crates/bitwarden-crypto/src/keys/prf.rs:9

let secret: [u8; 32] = secret.try_into().expect("length to be 32 bytes");

This expect() is actually fine because:

• We just validated length with split_at_checked(32)
• The compiler can't prove the length statically
• This is defensive programming

However, you could use unwrap_unchecked() in a production path for micro-optimization, but it requires unsafe. The current approach is good.

Good Practices Observed

• ✅ Proper use of Box::pin() for sensitive key material (prevents accidental copies)
• ✅ Comprehensive test coverage for key set rotation logic
• ✅ Clear separation of concerns (PRF derivation separate from key set management)
• ✅ Proper UniFFI integration for mobile client support
• ✅ Good use of Rust type system to prevent misuse (e.g., restricting to SymmetricCryptoKey)
• ✅ Followed deprecation patterns correctly (using dangerous_get_symmetric_key)

Action Items for Author

Must Address Before Merge:

1. ❌ Fix test_long_prf_truncated_to_proper_length test (Issue Dependency Dashboard #1)
2. ❌ Add module-level documentation explaining crypto algorithm rationale (Issue [PM-13910] Add collection manage property #4)

Should Address:
3. ⚠️ Consider constant-time comparison for zero-check or document why timing attack is not a concern (Issue #3)
4. ⚠️ Add test cases for PRF boundary conditions (Issue #2)

Nice to Have:
5. 🎨 Consider improving error messages for invalid PRF length (Issue #5)
6. 🎨 Consider creating Jira issues for TODO items or removing dead code (Issue #6)

Review Status

Overall Assessment: The implementation is well-structured and follows Rust best practices. The core cryptographic approach appears sound (based on existing TypeScript implementation). However, there are test quality issues and missing documentation that should be addressed before merging.

Recommendation: Request changes for items #1 and #4 (critical), author's discretion on remaining items.

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

I rebased and incorporated the changes mentioned by Bernd, squashed the lint fixes, and added some other commits to make CI all happy.

This is ready for review. (I updated the Jira ticket to PM-26354 to separate it from the mobile work.)

Looks like there are some new merge conflicts, other than that LGTM.

Addressed feedback, and opened #537 to address failing test coverage step in CI.